Implement dynamic import.defer() (TC39 Stage 3)#31244
Conversation
|
Updated 5:53 PM PT - Jun 18th, 2026
❌ @robobun, your commit c7d8bce has 3 failures in
🧪 To try this PR locally: bunx bun-pr 31244That installs a local version of the PR into your bun-31244 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (10)
WalkthroughAdds end-to-end support for TC39 Stage 3 dynamic ChangesDynamic Import Deferred Evaluation
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/resolve/import-defer.test.ts`:
- Around line 668-669: Move the assertion that checks stderr content before the
assertion that checks the exit code in the syntax-error tests: replace the
pattern expect(exitCode).not.toBe(0);
expect(stderr.toLowerCase()).toContain("error"); with
expect(stderr.toLowerCase()).toContain("error"); expect(exitCode).not.toBe(0);
for each occurrence (the three places that currently use those two assertions).
Ensure you only swap the order of the two existing assertions (leave the exact
match/transform on stderr and the not-to-be(0) check unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cedd6a09-87a3-43a7-afcc-dc584d6c0f45
📒 Files selected for processing (15)
src/ast/e.rssrc/ast/expr.rssrc/ast/import_record.rssrc/js_parser/p.rssrc/js_parser/parse/parse_import_export.rssrc/js_parser/parser.rssrc/js_parser/repl_transforms.rssrc/js_parser/visit/visit_expr.rssrc/js_printer/lib.rssrc/jsc/bindings/NodeVM.cppsrc/jsc/bindings/NodeVM.hsrc/jsc/bindings/ZigGlobalObject.cppsrc/runtime/bake/BakeGlobalObject.cpptest/bundler/bundler_edgecase.test.tstest/js/bun/resolve/import-defer.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing both nits — I don't see any remaining issues, but given this threads a new TC39 phase through the parser, printer, and three C++ moduleLoaderImportModule hooks, it's worth a human pass before merging.
Extended reasoning...
Overview
This PR implements dynamic import.defer() (TC39 Stage 3 deferred module evaluation), completing the static import defer work from #30975. It touches 15 files across four layers: AST (E::Import.phase_defer, ImportRecordFlags::PHASE_DEFER extended to dynamic records), parser (parse_import_expr accepts import.defer(...), threads phase_defer through TransposeState/transpose_import), printer (emits import.defer( for external/non-literal records, degrades to plain import( when WRAP_WITH_TO_ESM would defeat it), and the C++ runtime where ZigGlobalObject, BakeGlobalObject, and NodeVM now forward the previously-dropped bool deferred into JSC::importModule. ~310 lines of new tests cover deferral semantics, TLA, error propagation, CJS interop, attributes, transpiler round-tripping, and bundler degradation.
Security risks
None identified. The change is parsing/threading a boolean phase flag for a standards-track syntax form; no auth, crypto, filesystem, or network surface is touched. The C++ changes replace hardcoded false with a forwarded parameter on an existing JSC hook signature.
Level of scrutiny
High. This is a new language-feature implementation that alters how the JS parser branches on import., adds a field to a core AST node, and changes module-loader host-hook behavior across three global-object variants. Subtle interactions (ESM-marker handling for import.meta vs import.defer, escaped-keyword rejection, bundler inlining vs external paths, code-splitting + __toESM interop) are exactly the kind of thing a maintainer should sanity-check against the spec and JSC's expectations.
Other factors
Both of my earlier inline nits (stale "meta" diagnostic, redundant .defer under WRAP_WITH_TO_ESM) and CodeRabbit's assertion-order nit have been addressed in 117a476, 015d3eb, and 8531a4f respectively. The bug-hunting system found no issues on the current revision. Test coverage is thorough (19 new runtime cases + 3 bundler cases) and the PR description documents verification including no-regression runs. Buildkite #57072 reports failures on an earlier commit; I'd defer to a human to confirm those are unrelated/resolved on the latest push.
|
Status for maintainers — the implementation and review cycle are done; the remaining red CI is unrelated flake.
|
sosukesuzuki
force-pushed
the
farm/d83170ad/dynamic-import-defer
branch
from
792fcd8 to
4519f76
Compare
|
Status for maintainers — rebased onto current main ( Build #63407: 284 jobs passed, 1 hard-failed lane (darwin-14 aarch64). Failures are
My one allowed retrigger is used, so this needs a maintainer to re-run the flaky darwin lane or merge past it. |
robobun
force-pushed
the
farm/d83170ad/dynamic-import-defer
branch
from
4519f76 to
8372138
Compare
Parse `import.defer(specifier[, options])` into `E::Import { phase_defer }`,
mark the dynamic import record with `PHASE_DEFER`, and print the call back
out as `import.defer(...)` so JSC sees the phase. The module loader host
hook now forwards the `deferred` flag it receives from JSC into
`JSC::importModule(...)` (Zig, Bake, and node:vm default-loader paths),
letting JSC link the graph, evaluate only async transitive dependencies, and
resolve with the deferred namespace.
When the bundler inlines the target module, the call degrades to a regular
dynamic import — the same documented limitation as static `import defer`.
With code splitting, a deferred dynamic import of a cross-chunk CommonJS target gets wrapped in .then((m)=>__toESM(m.default)), which touches the namespace immediately and would defeat the defer phase on the same line. Emit a regular dynamic import there instead.
robobun
force-pushed
the
farm/d83170ad/dynamic-import-defer
branch
from
8372138 to
c7d8bce
Compare
1 participant
Implements the dynamic form of the Stage 3 Deferred Module Evaluation proposal, completing the feature started in #30975 (static
import defer):Bun's pinned WebKit already ships the engine side (upstream WebKit/WebKit#64907): the JSC parser accepts
import.defer(...)underuseImportDefer(which Bun enables), and themoduleLoaderImportModulehost hook carries a trailingbool deferred. Bun previously rejected the syntax in its own transpiler and ignored that flag.What changed
Parser / AST —
src/js_parser/parse/parse_import_export.rs,src/ast/e.rs,src/js_parser/{parser,p,visit/visit_expr,repl_transforms}.rsimport.now accepts the contextual keyworddeferfollowed by the usual( specifier [, options] )arguments. The check compares the raw token, soimport.def\u0065r(...)stays a syntax error; other identifiers afterimport.still error.import.meta,import.defer()does not mark the file as ESM (it is valid in CommonJS, likeimport()).E::Import.phase_defer, threaded through import transposition (TransposeState.phase_defer), andImportRecordFlags::PHASE_DEFERis now also set onImportKind::Dynamicrecords.import defer * as X from 'mod'→await import.defer('mod').Printer —
src/js_printer/lib.rsimport.defer(...), so JSC's own parser sees the phase during bytecode generation.Promise.resolve().then(...)) — the same documented limitation as staticimport defer. The bake dev server'shmr.dynamicImportpath is likewise unchanged.Runtime —
src/jsc/bindings/ZigGlobalObject.cpp,src/runtime/bake/BakeGlobalObject.cpp,src/jsc/bindings/NodeVM.{h,cpp}moduleLoaderImportModuleforwards thedeferredflag intoJSC::importModule(...)instead of dropping it, for the main global, Bake global, plugin virtual modules, and the node:vmUSE_MAIN_CONTEXT_DEFAULT_LOADERpath. JSC then links the graph, eagerly evaluates only the async transitive dependencies, and resolves the promise with the deferred namespace (ensureDeferredNamespaceEvaluationhandles later property access).importModuleDynamicallycallbacks have no phase parameter (same as Node), so deferred imports resolved through a user callback behave like regular dynamic imports.Semantics covered by tests (
test/js/bun/resolve/import-defer.test.ts, 19 new cases)ns[Symbol.toStringTag] === "Deferred Module"import.defer()and staticimport deferof the same module yield the same namespace object.tsfiles, CommonJS callers, CommonJS targets (host-defined eager evaluation)Bun.Transpilerround-tripsimport.defer(...)andscanImportsreports it asdynamic-importimport.deferwithout a call, escapeddef\u0065r,import.source(...)Bundler coverage (
test/bundler/bundler_edgecase.test.ts): inlined targets degrade gracefully and run;--externaltargets keepimport.defer(...)in the output.Verification
bun bd test test/js/bun/resolve/import-defer.test.ts— 39 pass (20 existing static + 19 new)bun bd test test/bundler/bundler_edgecase.test.ts -t DynamicImportDefer— 2 passUSE_SYSTEM_BUN=1(1.4.0 rejects the syntax withExpected "meta" but found "defer")transpiler.test.js,vm.test.ts,repl-transform.test.ts,concurrent-dynamic-import,dynamic-import-tla-cycle,import-meta,import-queryall pass